Skip to content

Fix recording shutdown for node docker#3106

Merged
VietND96 merged 6 commits intotrunkfrom
fix-video-shutdown
Apr 9, 2026
Merged

Fix recording shutdown for node docker#3106
VietND96 merged 6 commits intotrunkfrom
fix-video-shutdown

Conversation

@VietND96
Copy link
Copy Markdown
Member

@VietND96 VietND96 commented Apr 3, 2026

Thanks for contributing to the Docker-Selenium project!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines, applied for this repository.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Centralize supervisord shutdown signaling to fix recording shutdown

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Centralize supervisord shutdown signaling in Python controller
• Prevent duplicate SIGTERM signals causing extended shutdown delays
• Handle external vs self-initiated shutdown scenarios correctly
• Add safeguards against re-entrant signal handling in shell mode
Diagram
flowchart LR
  A["video_recorder.py"] -->|"Captures external shutdown"| B["Signal handlers"]
  B -->|"SIGTERM/SIGINT"| C["Mark external shutdown"]
  A -->|"Self-initiated exit"| D["_signal_supervisord"]
  D -->|"SIGTERM"| E["supervisord"]
  F["video.sh graceful_exit_force"] -->|"Delegates to controller"| A
  G["Shell recorder"] -->|"Forward signal once"| H["video.sh"]
  H -->|"Exit normally"| D
Loading

Grey Divider

File Changes

1. Video/video.sh 🐞 Bug fix +7/-1

Remove supervisord signaling from shell script

• Add guard flag _graceful_exit_done to prevent re-entrant calls to graceful_exit_force
• Remove direct supervisord SIGTERM signaling from shell script
• Add comment explaining supervisord signaling is delegated to Python controller
• Simplify shutdown logic by removing duplicate signal handling

Video/video.sh


2. Video/video_recorder.py 🐞 Bug fix +61/-4

Centralize supervisord signaling and fix shutdown logic

• Add _signal_supervisord() function to centralize supervisord shutdown logic
• Capture external shutdown state before asyncio replaces signal handlers in event-driven mode
• Only signal supervisord for self-initiated exits, not external shutdowns
• Prevent re-entrant signal forwarding in shell mode to avoid SIGTERM ping-pong
• Add comprehensive documentation explaining shutdown behavior and container lifecycle

Video/video_recorder.py


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review bot commented Apr 3, 2026

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX Issues (0)

Grey Divider


Action required

1. SIGTERM swallowed pre-async🐞
Description
In event-driven mode, main() installs SIGTERM/SIGINT handlers that only set a flag and return, so a
stop request received before asyncio.run() installs loop signal handlers will not terminate or
initiate shutdown and can hang until supervisord SIGKILLs the process.
Code

Video/video_recorder.py[R48-57]

+        # Capture whether shutdown was externally initiated (SIGTERM/SIGINT)
+        # before asyncio.run() replaces the signal handlers via add_signal_handler.
+        _external_shutdown = [False]
+
+        def _mark_external_shutdown(signum, frame):
+            _external_shutdown[0] = True
+
+        signal.signal(signal.SIGTERM, _mark_external_shutdown)
+        signal.signal(signal.SIGINT, _mark_external_shutdown)
+
Evidence
video_recorder.py replaces SIGTERM/SIGINT with _mark_external_shutdown that does not exit or trigger
shutdown, and video_service.py only installs the real shutdown handlers later via
loop.add_signal_handler. Because the supervisord program uses stopsignal=TERM with a 60s
stopwaitsecs, a TERM during this window can prevent graceful stop and force a kill.

Video/video_recorder.py[44-57]
Video/video_service.py[997-1004]
Video/recorder.conf[1-10]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
In `SE_VIDEO_EVENT_DRIVEN=true` mode, `video_recorder.py` installs temporary SIGTERM/SIGINT handlers that only set a flag and return. If supervisord (or the user) sends SIGTERM during startup (before the asyncio loop handlers in `video_service.py` are installed), the process will ignore the stop request and keep running until supervisord hits `stopwaitsecs` and SIGKILLs.

### Issue Context
The event-driven service later installs its own handlers using `loop.add_signal_handler(...)`, but those are not active during the early startup window.

### Fix Focus Areas
- Video/video_recorder.py[48-57]

### Suggested fix
Update `_mark_external_shutdown` to *also* preserve termination semantics during the pre-async window. For example:
- For SIGTERM: set the flag then `raise SystemExit(0)` (or re-raise the signal after restoring `SIG_DFL`).
- For SIGINT: set the flag then call `signal.default_int_handler(signum, frame)`.
This keeps the flag behavior while ensuring SIGTERM/SIGINT still stop the process when received before asyncio installs its handlers.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Event-driven shutdown misclassified🐞
Description
In event-driven mode, SIGTERM/SIGINT delivered after the asyncio loop starts are handled by
video_service.py (loop.add_signal_handler), leaving _external_shutdown false and causing
video_recorder.py to treat an external stop as self-initiated and signal supervisord redundantly.
Code

Video/video_recorder.py[R71-73]

+        # Only trigger container shutdown for self-initiated exits (drain).
+        if not _external_shutdown[0]:
+            _signal_supervisord()
Evidence
video_recorder.py decides whether to call _signal_supervisord() based on _external_shutdown, but
video_service.py replaces SIGTERM/SIGINT handling with loop.add_signal_handler that does not set
that flag. As a result, an externally-triggered stop during normal event-driven operation can still
follow the "self-initiated" path in video_recorder.py.

Video/video_recorder.py[48-57]
Video/video_recorder.py[71-73]
Video/video_service.py[997-1004]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
In event-driven mode, `_external_shutdown` is only set by the temporary `signal.signal(...)` handlers in `video_recorder.py`. Once `video_service.py` installs `loop.add_signal_handler(...)`, SIGTERM/SIGINT no longer update `_external_shutdown`, so `video_recorder.py` may call `_signal_supervisord()` even when shutdown was initiated by supervisord.

### Issue Context
This is redundant (and possibly confusing) signaling: supervisord already initiated shutdown.

### Fix Focus Areas
- Video/video_recorder.py[48-57]
- Video/video_recorder.py[71-73]

### Suggested fix
Make event-driven signal handling set the external-shutdown flag in the *active* handlers:
- Option A: Modify `video_service.main()` to accept a callback/flag-setter, and have the `loop.add_signal_handler` lambda set both `external_shutdown=True` and `service.shutdown_event.set()`.
- Option B: In `video_recorder.py`, avoid the temporary `signal.signal` approach and instead have `service_main()` return an exit reason (e.g., `"external_signal"` vs `"drain"`) so the controller can decide whether to signal supervisord.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

VietND96 added 3 commits April 5, 2026 11:18
Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
@VietND96 VietND96 force-pushed the fix-video-shutdown branch from b9f2d2a to 35b08dc Compare April 7, 2026 18:45
Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
@VietND96 VietND96 force-pushed the fix-video-shutdown branch from 35b08dc to 385acbd Compare April 8, 2026 01:14
Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
@VietND96 VietND96 merged commit 01efc95 into trunk Apr 9, 2026
79 of 85 checks passed
@VietND96 VietND96 deleted the fix-video-shutdown branch April 9, 2026 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant